fix(ecs): stack name can result in noncompliant capacity provider name#36584
fix(ecs): stack name can result in noncompliant capacity provider name#36584
Conversation
Previsouly fixed with aws#29235 but only for lower case stack names. The error could still be reproduced with stack names starting with `Ecs` or `AWS`, etc. ``` const app = new cdk.App(); const stack = new cdk.Stack(app, 'EcsCp'); const vpc = new ec2.Vpc(stack, 'Vpc'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); const autoScalingGroupAl2 = new autoscaling.AutoScalingGroup(stack, 'asgal2', { vpc, instanceType: new ec2.InstanceType('bogus'), machineImage: ecs.EcsOptimizedImage.amazonLinux2(), }); const capacityProvider = new ecs.AsgCapacityProvider(stack, 'provideral2-2', { autoScalingGroup: autoScalingGroupAl2, enableManagedTerminationProtection: false, }); ``` Deploying the code above will result in: ``` Resource handler returned message: "Invalid request provided: CreateCapacityProvider error: The specified capacity provider name is invalid. Up to 255 characters are allowed, including letters (upper and lowercase), numbers, underscores, and hyphens. The name cannot be prefixed with "aws", "ecs", or "fargate". Specify a valid name and try again. (Service: Ecs, Status Code: 400, Request ID: 6c28bb07-463b-497a-9e18-046f8ef58de0) (SDK Attempt Count: 1)" (RequestToken: 2e19581f-7343-6fda-588e-94cbe50066d1, HandlerErrorCode: InvalidRequest) ``` Fixes aws#29151
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request: There is already an integration test for capacity provider. The unit test should suffice here. |
aws-cdk-automation
added
the
pr-linter/exemption-requested
aws-cdk-automation
added
the
pr/needs-community-review
lpizzinidev
left a comment
lpizzinidev
left a comment
There was a problem hiding this comment.
Ty! Left a couple of notes.
lpizzinidev
left a comment
lpizzinidev
left a comment
There was a problem hiding this comment.
Ty. Nit on a missing unit test case.
Extra: It would be nice to add a new integration test to cover the scenario where the cluster name starts with aws|ecs|fargate and the cp- prefix is added automatically before deploy as I don't see it covered with the original fix.
That would also satisfy automation checks.
Eg
const cluster = new ecs.Cluster(stack, 'EcsCluster');
...
const capacityProvider = new ecs.AsgCapacityProvider(stack, 'provideral2-2', {
.. // props without specifying `capacityProviderName`
});
|
Sure, I can try and add integration tests. Never done it before in this repo, but I'm sure it's very doable. But are we sure it adds any value in this case? The unit tests already cover that specific use case. This one, for example, confirms the name will have the |
lpizzinidev
left a comment
lpizzinidev
left a comment
There was a problem hiding this comment.
Ty.
But are we sure it adds any value in this case?
Fair point. We don't have a dedicated integration test to check that a stack with aws|ecs|fargate prefix actually deploys the capacity provider with cp- prefix but this should provide the necessary coverage.
A couple of comments needs to be adjusted but I am good with not adding the integration test.
|
@lpizzinidev anything else I should do? |
|
@kichik Nope, just waiting for a maintainer's review |
3 participants
Previously fixed with #29235 but only for lower case stack names. The error could still be reproduced with stack names starting with
EcsorAWS, etc.Issue # (if applicable)
Closes #29151
Reason for this change
Deploying the code above will result in:
Description of changes
The regular expression testing for "bad" prefixes was adjusted to be case-insensitive.
Describe any new or updated permissions being added
No new permissions.
Description of how you validated changes
Unit tests were updated to confirm the issue and then the code was fixed and the new unit test passed.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license